Android native: tighten Oboe build, fix getLastError race, cut audio-thread syscalls - #208
Conversation
Agent-Logs-Url: https://github.com/winnerspiros/osu/sessions/bfd3fec5-2132-4425-949d-b693a7eee8ba Co-authored-by: winnerspiros <1675249+winnerspiros@users.noreply.github.com>
Agent-Logs-Url: https://github.com/winnerspiros/osu/sessions/9b5aafe3-d606-42af-93bc-8af83d6a31f2 Co-authored-by: winnerspiros <1675249+winnerspiros@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR optimizes and hardens the Android native audio integration by reducing unnecessary Oboe build work, lowering audio-thread syscall pressure, and fixing a race in the native-to-managed error-message path.
Changes:
- CMake: disable Oboe tests/examples/docs and use shallow FetchContent to speed up CI/build; adjust Oboe compile options to avoid problematic flags.
- Audio callback: throttle
updateLatency()to every 256 callbacks while keepingLatencyTuner::tune()at 128. - Error ABI hardening: make
getLastError()return by value (copy under lock) and return a stablethread_localsnapshot fromnOboeGetLastErrorMessage.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| osu.Android/Native/CMakeLists.txt | Disables non-shipping Oboe artifacts, shallow-fetches Oboe, and patches Oboe compile flags for compatibility. |
| osu.Android/Native/oboe_bridge.h | Changes getLastError() to return std::string by value with updated contract comment. |
| osu.Android/Native/oboe_bridge.cpp | Fixes AllowedCapturePolicy enum, reduces audio-thread syscalls, and makes last-error export race-safe via thread_local snapshot. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Explicit dependency for gettid() used in nADPFCreateSession below — do not | ||
| // rely on transitive includes from Oboe / NDK headers, which may change. | ||
| #include <unistd.h> |
There was a problem hiding this comment.
<unistd.h> is already included at the top of this file, so the new mid-file #include <unistd.h> is redundant and the “don’t rely on transitive includes” comment is now misleading. Consider removing the duplicate include (and ideally keeping all includes in one place).
| // Explicit dependency for gettid() used in nADPFCreateSession below — do not | |
| // rely on transitive includes from Oboe / NDK headers, which may change. | |
| #include <unistd.h> |
| /// Returns a copy of the most recent error message under lock. We return | ||
| /// by value (not a pointer to internal storage) so callers can't observe a | ||
| /// torn or freed `std::string` if another thread mutates `lastError_` | ||
| /// concurrently (Oboe error callbacks fire from an internal thread). | ||
| std::string getLastError() const; |
There was a problem hiding this comment.
oboe_bridge.h uses std::string in the public API (getLastError()) but the header doesn’t explicitly include <string>. It currently works only if a transitive include happens to pull it in; please add #include <string> to make the header self-contained.
|
|
||
| uint32_t count = callbackCount_.fetch_add(1, std::memory_order_relaxed); | ||
|
|
||
| // LatencyTuner once every 128 callbacks (~1.5s @ 192 burst, 48 kHz). |
There was a problem hiding this comment.
The new comment says “~1.5s @ 192 burst, 48 kHz”, but 128 callbacks * 192 frames / 48kHz is ~0.51s (and the callback frame count may not even be fixed since framesPerDataCallback is unspecified). Please correct or soften the timing estimate to avoid misleading future readers.
| // LatencyTuner once every 128 callbacks (~1.5s @ 192 burst, 48 kHz). | |
| // Run the LatencyTuner once every 128 callbacks. The wall-clock interval | |
| // depends on the callback frame count and sample rate (for example, at | |
| // 192 frames/callback and 48 kHz this is about 0.5s). |
Follow-up to the Oboe/Veldrid integration: apply the deferred optimizations from the previous PR and harden one race-y P/Invoke path discovered on a second pass. No game logic touched.
CMake (
osu.Android/Native/CMakeLists.txt)-ffast-mathfrom Oboe's compile options (kept on our own bridge). Avoids__FINITE_MATH_ONLY__symbol-version drift against system libm.OBOE_BUILD_TESTS / EXAMPLES / DOCS = OFFandBUILD_TESTING = OFF— FetchContent stops compiling artifacts we never ship.GIT_SHALLOW TRUEon the OboeFetchContent_Declarefor faster CI checkout.Audio bridge (
oboe_bridge.{h,cpp})#include <unistd.h>next to<android/performance_hint.h>sogettid()no longer rides on transitive Oboe/NDK includes.updateLatency()to every 256 callbacks while leavingLatencyTuner::tune()at 128 — halves AAudio syscall pressure on the audio thread.nOboeGetLastErrorMessage: previous code returnedlastError_.c_str()after releasing the mutex, so a concurrent Oboe error callback could mutate thestd::stringbefore the P/Invoke marshaller copied it.getLastError()now returns by value (copy under lock); the C export stages it into athread_localsnapshot whose lifetime covers the marshal step. Managed ABI unchanged.Not touched
vulkan_bridge.cpp(runs once at startup), the C# wrapper, and both submodule forks — none need changes for these wins.